Skip to content

Conversation

@RicardCForgeFlow
Copy link
Contributor

Totals were only recomputed on initial render or after saving because the renderer cached matrix/rows/columns and only rebuilt them on props updates.

2026-01-15.11-46-33.mp4

Compute matrix data from reactive list.records on each render so row/column/grand totals update immediately when a cell value changes.

2026-01-15.11-44-07.mp4

@OCA-git-bot
Copy link
Contributor

Hi @JasminSForgeFlow, @DavidJForgeFlow, @hbrunn,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@JasminSForgeFlow JasminSForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review 👍

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't do this. The widget as is is already pretty expensive computationally, and this PR makes it much worse.

Please replace all of this with

        onWillRender(() => {
            this.columns = this._getColumns();
            this.rows = this._getRows();
            this.matrix = this._getMatrix();
        });

in setup() instead of onWillUpdateProps, this should get the caching right and won't add several times o(n^2) complexity.

Also consider expanding the test to edit a field several times and check if the totals are updated correctly

@RicardCForgeFlow
Copy link
Contributor Author

don't do this. The widget as is is already pretty expensive computationally, and this PR makes it much worse.

Please replace all of this with

        onWillRender(() => {
            this.columns = this._getColumns();
            this.rows = this._getRows();
            this.matrix = this._getMatrix();
        });

in setup() instead of onWillUpdateProps, this should get the caching right and won't add several times o(n^2) complexity.

Also consider expanding the test to edit a field several times and check if the totals are updated correctly

Okay!

@RicardCForgeFlow RicardCForgeFlow marked this pull request as draft January 16, 2026 09:02
@RicardCForgeFlow RicardCForgeFlow force-pushed the 18.0-fix-web_widget_x2many_2d_matrix-compute branch 2 times, most recently from cb87b79 to ecbd1a0 Compare January 16, 2026 10:53
Totals were not consistently refreshing immediately on cell edits because the
renderer logic relied on expensive getters that caused redundant computations
per render cycle and lacked proper reactivity.
- Refactor X2Many2DMatrixRenderer to use the `onWillRender` hook, computing
  all matrix data (rows, columns, and cells) once per render cycle.
  This ensures totals update immediately when a cell value changes and
  significantly improves performance.
- Expand the JS test suite with a robust case verifying totals after
  multiple consecutive edits.
- Adapt tests to the Odoo 18 Hoot framework using `contains` and `queryAll`.
@RicardCForgeFlow RicardCForgeFlow force-pushed the 18.0-fix-web_widget_x2many_2d_matrix-compute branch from ecbd1a0 to 64dce27 Compare January 16, 2026 12:36
@RicardCForgeFlow RicardCForgeFlow marked this pull request as ready for review January 16, 2026 12:54
@RicardCForgeFlow
Copy link
Contributor Author

don't do this. The widget as is is already pretty expensive computationally, and this PR makes it much worse.

Please replace all of this with

        onWillRender(() => {
            this.columns = this._getColumns();
            this.rows = this._getRows();
            this.matrix = this._getMatrix();
        });

in setup() instead of onWillUpdateProps, this should get the caching right and won't add several times o(n^2) complexity.

Also consider expanding the test to edit a field several times and check if the totals are updated correctly

Hi @hbrunn I changed the onWillUpdateProps for your code. Also added a test with the goal of replicating the problem. Please tell me if you see everything correct. Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants